Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn user if cargo ament build is not installed #31

Merged

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Nov 14, 2024

This PR returns an error if cargo-ament-build was not found, including a message on how to install it, it's pretty straightforward but a few notes:

  • I opted to recommend for using cargo install cargo-ament-build as noted in the ros2-rust README, I thought about recommending pip but then it would cause a lot of trouble on 24.04 because virtual envs etc.
  • I thought of falling back to a colcon-cargo package but I'm not 100% convinced it is a good idea to silently continue building with a build type different from what the users might expect, instead of loudly printing an error and failing.
  • The failure method of this package identification is a bit odd and I'm not 100% sure it is the right thing to do, specifics below in the Test it! section.

Test it!

  • Copy the sample package into a colcon workspace
  • Try a colcon build with cargo ament-build installed
  • Remove cargo-ament-build
  • Try again

You should see the (as I mentioned above slightly strange) failure:

lucadv@ros:~/colcon_ws$ colcon build
[0.241s] ERROR:colcon.colcon_ros_cargo.package_identification.ament_cargo:ament_cargo package found but cargo ament-build was not detected. Please install it by running: `cargo install cargo-ament-build`
[0.332s] WARNING:colcon.colcon_core.verb:No task extension to 'build' a 'ros.ament_cargo' package

Specifically the second warning is a bit odd but it will happen in all our error paths, so we should probably do something more than early returning on failure, but that is something out of scope for this specific PR

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 12 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@a1c4900). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...on_ros_cargo/package_identification/ament_cargo.py 20.00% 11 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #31   +/-   ##
=======================================
  Coverage        ?   60.54%           
=======================================
  Files           ?        8           
  Lines           ?      403           
  Branches        ?       53           
=======================================
  Hits            ?      244           
  Misses          ?      149           
  Partials        ?       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the --help flag to test if the program exists is very clever.

I left a very minor recommendation for tweaking the way the information is presented, but I think the PR is good either way.

colcon_ros_cargo/package_identification/ament_cargo.py Outdated Show resolved Hide resolved
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Tweaking the way the warning is displayed
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I think this will really help users to navigate the unusual Rust dependency.

Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luca-della-vedova luca-della-vedova merged commit 182fcab into colcon:main Nov 25, 2024
16 checks passed
@luca-della-vedova luca-della-vedova deleted the luca/warn_cargo_ament branch November 25, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants